-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#349] Add service name generator to be able to run tests concurrently #472
[#349] Add service name generator to be able to run tests concurrently #472
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to do the exact same thing in the RMW 😆 .
When
Which then lead to a failure when opening the dynamic service info and then the builder assumes the services is in a corrupted state. The mechanism that seems to fail here is the concurrent dynamic storage creation/opening for the
When the opener comes it first checks if it has read/write permissions, if not, initialization is still in progress. If it has read/write permissions it also checks the version number. If it is zero, still in initialization, if not zero it must match its own local version number. |
…hecked for changes instead of reloading the underlying memory
Okay, there was an actual bug in the dynamic storage posix shared memory implementation. When the creation process is still initializing the memory, the opener takes a COPY of the version number instead of a pointer to it and checks it until it becomes non-zero. The thing is, this is a bug that should only occur on OSes that do not support adjusting the permissions of the shm during runtime like Mac OS and Windows and not linux. And this also does not fix the failing test |
…es CARGO_PKG_VERSION_{MAJOR|MINOR|PATCH} all to 0 by setting them to u16::MAX
ac74d05
to
7f662b9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #472 +/- ##
==========================================
- Coverage 78.93% 78.92% -0.01%
==========================================
Files 197 198 +1
Lines 23637 23627 -10
==========================================
- Hits 18657 18647 -10
Misses 4980 4980
|
} | ||
|
||
PackageVersion::from_u64(PACKAGE_VERSION.load(Ordering::Relaxed)) | ||
PackageVersion::from_version(MAJOR, MINOR, PATCH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious, do we have a specific version convention? not sure why we didn't use the semver, which is popular in rust. so in the future we may have beta
, alpha
or rc
versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general we try to be very cautious to introduce external crates. For any other project I would agree with you but iceoryx2 shall be certified as ASIL D according to ISO26262 which is the highest safety standard in the automotive world.
This requires 100% MC/DC coverage for every line of code. Every test must be covered by a requirement, we need detailed architecture documents and documentation, a safety manual and so on.
So, every dependency we introduce must satisfy all of this! No exception. But actually, nearly none of them does it, and it is a challenge to do it. We have a strategy in place how to solve this but one of the key factors is - try to avoid external crates as much as possible.
Another answer we need to find is, how to handle or prevent supply chain attacks.
So in light of all of this, we try to be very cautious with external dependencies since the more we have the more work we have to do in the near future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ASIL D according to ISO26262 ... how to handle or prevent supply chain attacks.
understood, quite a bit different from the normal project. thanks for your explaination:)
@@ -31,6 +32,15 @@ using ServiceTypeLocal = TypeServiceType<ServiceType::Local>; | |||
|
|||
using ServiceTypes = ::testing::Types<ServiceTypeIpc, ServiceTypeLocal>; | |||
|
|||
inline auto generate_service_name() -> ServiceName { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like it deserves a unit test:)
@@ -79,10 +79,11 @@ number is `Xold.Yold.Zold`. | |||
10. Adjust the `<version>` to `X.Y.Z` in `$GIT_ROOT$/package.xml`. | |||
11. Call `rg "Xold\.Yold\.Zold"` and adjust all findings. | |||
* C and C++ examples, `BUILD.bazel` & `CMakeLists.txt` | |||
12. **Merge all changes to `main`.** | |||
13. Set tag on GitHub and add the release document as notes to the tag | |||
12. Adjust the major, minor and patch version number in `iceoryx2_bb_elementary::PackageVersion` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a script taking care of setting the version number in all known locations would be helpful. I'll add it to my todo list :)
Notes for Reviewer
Fixes some parts of the problem but not everything
Pre-Review Checklist for the PR Author
SPDX-License-Identifier: Apache-2.0 OR MIT
iox2-123-introduce-posix-ipc-example
)[#123] Add posix ipc example
)task-list-completed
)Checklist for the PR Reviewer
Post-review Checklist for the PR Author
References
Relates to #349
Closes #475